@ClaudeWeb review this pr

Review: IDfy CRC API Integration

The integration is structurally sound and consistent with the existing IDfy verification patterns (RC, DL, PAN, Udyam). No security issues found — API keys are properly decrypted via decrypt, PII fields are passed through correctly. A few things to address before merging.

Warning — GetTaskResp is a breaking change for all downstream consumers

File: lib/mobility-core/src/Kernel/External/Verification/Interface/Types.hs

Adding CRCResp to the GetTaskResp sum type means every pattern match on GetTaskResp without a wildcard catch in nammayatri/nammayatri will now be non-exhaustive. Please confirm all call sites are updated alongside this PR or in an atomic follow-up.

If GetTaskResp values are ever serialised and stored (Redis, job queue, etc.), old payloads without the "CRCResp" tag will fail to deserialise after this ships.

Warning — Untyped status and risk_type fields (confidence 80)

File: lib/mobility-core/src/Kernel/External/Verification/Types.hs

status :: Maybe Text and risk_type :: Maybe Text are the primary fields callers will branch on. IDfy returns a finite set of values for both ("completed" / "in_progress" / "failed" and "low" / "medium" / "high"). As free-form Text, a misspelled or unexpected value from the API silently produces wrong downstream behavior.

Suggested fix — model them as ADTs with a fallback:

data CRCStatus = CRCCompleted | CRCInProgress | CRCFailed | CRCStatusUnknown Text
  deriving (Show, Generic, ToSchema)

data CRCRiskType = CRCLow | CRCMedium | CRCHigh | CRCRiskUnknown Text
  deriving (Show, Generic, ToSchema)

Same concern applies to report_mode :: Maybe Text in the request type — if the IDfy API accepts only a fixed set of modes, this should be an ADT.

Warning — No tests added (confidence 85)

convertCRCOutputToCRCVerification is a pure function with no external dependencies — it's trivially testable. A unit test covering the happy path (all fields populated) and the all-Nothing degenerate case would catch any field-mapping mistakes (e.g., riskSummary vs risk_summary mixups). No other conversion functions in this file have tests either, but this PR is a good place to start.

Potential — dob :: Maybe Text format undocumented (confidence 65)

File: lib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Request.hs

IDfy typically expects dob in DD-MM-YYYY format. An incorrect format will fail silently at the type level and only surface as a 4xx at runtime. At minimum add a comment:

dob :: Maybe Text  -- Expected format: DD-MM-YYYY

A newtype Dob = Dob Text with a smart constructor would be stronger.

Potential — entity_type hardcoded magic string (confidence 65)

File: lib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hs (~line 228)

entity_type = "individual" is hardcoded. Since ind_court_record is individual-only per the IDfy docs, this is safe today. CodeRabbit already flagged this. If the intent is always individual, consider:

data CRCEntityType = Individual

instance ToJSON CRCEntityType where
  toJSON Individual = "individual"

Even a one-constructor ADT removes the magic string and makes the intent explicit.

Process — PR description checklist is empty

All checklist items are unchecked. Please tick off formatting/lint check, self-review, and tests (or mark N/A), and add a CHANGELOG.md entry for the new capability.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/mobility-core/src/Kernel/External/Verification/Interface/Types.hs`:
- Around line 289-290: You added a new constructor CRCResp to the public sum
type GetTaskResp which is a breaking change; revert that constructor addition
from GetTaskResp and instead introduce a new versioned type (e.g. GetTaskRespV2)
that duplicates all current constructors plus CRCResp, or create a separate
CRC-specific wrapper type, and update only the new consumers/parsers to use
GetTaskRespV2 while leaving existing code using GetTaskResp untouched; reference
the GetTaskResp and CRCResp symbols when making the change so all pattern
matches and FromJSON/ToJSON instances remain stable for downstream code.

---

Nitpick comments:
In `@lib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Response.hs`:
- Around line 70-72: The new "ind_court_record" branch in Response.hs (the
parseJSON @(IdfyResponse (SourceOutput VT.CRCVerificationResponse)) path that
maps with mapIdfyResponse CRCResult) lacks unit tests; add focused tests that
(1) decode a valid ind_court_record JSON into IdfyResponse (SourceOutput
VT.CRCVerificationResponse) and round-trip encode-back to assert equality
(happy-path), and (2) decode cases where the result field is missing/null to
ensure the Nothing handling path of CRCResult is exercised and asserted; place
tests next to other Idfy Response specs and reference parseJSON, IdfyResponse,
CRCResult and the ind_court_record case so the branch at lines around the
parseJSON mapping is covered.

In `@lib/mobility-core/src/Kernel/External/Verification/Types.hs`:
- Around line 194-201: Change CRCVerificationResponse to use ADTs for the
enum-like fields: replace the riskType :: Maybe Text and status :: Maybe Text
with types like RiskType and VerificationStatus (each defined as concrete
constructors for known values plus an Unknown Text fallback, and keep a Maybe
wrapper if the whole field can be absent). Implement FromJSON and ToJSON (or
parsing functions) for RiskType and VerificationStatus so known strings map to
specific constructors and any unrecognized string maps to Unknown with the
original text preserved. Update any code that constructs or pattern-matches on
CRCVerificationResponse to use the new ADTs (e.g., pattern-match on
RiskType/VerificationStatus constructors instead of raw Text) and add conversion
helpers if needed to minimize call-site changes; ensure tests/parsers that
consume these fields are updated accordingly.

Review: Error Handling Deep-Dive (second pass)

I read the full source of Interface/Idfy.hs, Idfy/Types/Response.hs, and Kernel/Utils/JSON.hs. Two issues are blocking; the rest are warnings.

Bug 1 — CRCEntityType JSON round-trip mismatch (confidence 95)

File: lib/mobility-core/src/Kernel/External/Verification/Types.hs

Generic deriving ToJSON/FromJSON on a sum type uses constructor names verbatim → serialises as "Individual" / "Business" (capitalised). But crcEntityTypeToText produces "individual" / "business" (lowercase). If VerifyCRCReq.entityType is ever stored as JSON (DB, job queue, audit log) and then read back, FromJSON will fail to parse the capitalised value. Silent deserialization failure, only surfacing at runtime.

Fix: Add explicit instances using constructorsToLowerOptions (already in Kernel.Utils.JSON):

instance ToJSON CRCEntityType where
  toJSON = A.genericToJSON constructorsToLowerOptions

instance FromJSON CRCEntityType where
  parseJSON = A.genericParseJSON constructorsToLowerOptions

This also makes crcEntityTypeToText redundant.

Bug 2 — Response.hs uses the public interface type directly, breaking layer separation (confidence 90)

File: lib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Response.hs

Every other IDfy verification follows a two-layer pattern:

• IDfy layer (Response.hs): native snake_case fields matching the wire format (DLVerificationOutput, PanVerificationOutput, etc.)
• Interface layer (Types.hs): public camelCase type converted in Interface/Idfy.hs

This PR skips the IDfy layer for CRC and instead puts constructorsWithSnakeCase JSON options on VT.CRCVerificationResponse — a public interface type — to match the wire format. Two concrete error-handling consequences:

a) Silent all-Nothing on field name drift. With native snake_case fields, a mismatched field name is a compile error. With constructorsWithSnakeCase, the mapping is implicit. If IDfy returns case_detail_link (no 's') instead of case_details_link, Aeson silently returns Nothing. Since every field is Maybe, the whole response parses successfully as all-Nothing — no error, no log, just empty data.

b) IDfy wire format leaks into the public type. If IDfy renames a field, VT.CRCVerificationResponse must change, breaking all downstream consumers.

Fix: Restore the two-layer pattern — add CRCVerificationOutput with native snake_case fields to Response.hs, add convertCRCOutputToCRCVerification in Interface/Idfy.hs, and remove constructorsWithSnakeCase from VT.CRCVerificationResponse in Types.hs.

Note: I confirmed in Kernel/Utils/JSON.hs that constructorsWithSnakeCase = defaultOptions { fieldLabelModifier = camelToSnakeCase } (via camelTo2 '_'). The field mapping itself is correct (caseDetailsLink → case_details_link). The issue is architectural.

Warning — getTask throws InternalError on every poll of an in-progress CRC task (confidence 80)

File: lib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hs

result <- resp.result & fromMaybeM (InternalError ("Missing result in getTask response: " <> show resp))

When IDfy task status is "in_progress", result is null. fromMaybeM throws InternalError on every poll until completion. This is pre-existing for DL/RC/PAN but is more acute for court records: IDfy's ind_court_record can take minutes to hours. Every poll in that window generates a 500/alert.

Minimal fix within this PR scope: check resp.status before fromMaybeM:

when (resp.status == "in_progress") $
  throwError $ InternalError "CRC verification in progress"
result <- resp.result & fromMaybeM (InternalError ...)

Warning — Business entity type has no guard for ind_court_record endpoint (confidence 75)

The ind_ prefix in IDfy's naming means individual-only. Passing entity_type: "business" will produce a 4xx from IDfy, propagating as an unhandled exception. Either remove Business from the ADT if unsupported, or add:

when (req.entityType == VT.Business) $
  throwError $ InternalError "Business entity type not supported for ind_court_record"

Potential — monitor_case removed from request (confidence 70)

IDfy's ind_court_record supports monitor_case :: Bool for ongoing monitoring of new case filings — likely a primary use case for periodic background checks on drivers. This was in the first revision and removed in the current one. If intentional, please add a comment explaining why.

Please resolve the claude comment

Re-review after latest commit

Bug 1 (CRCEntityType JSON mismatch) — Fixed ✓

CRCEntityType now has explicit ToJSON/FromJSON/ToSchema instances using constructorsToLowerOptions. The round-trip is now consistent: both serialization and the wire format use lowercase "individual" / "business". Good fix.

Minor follow-up: crcEntityTypeToText is now redundant — constructorsToLowerOptions already produces the same lowercase strings. Not blocking, but the function can be removed and callers can use toJSON or a direct pattern match instead. Keeping it isn't wrong, just dead weight.

Bug 2 (layer separation) — Accepting the design, one residual concern remains

Understood — using constructorsWithSnakeCase directly in parseJSON is an intentional design choice to avoid a separate IDfy-layer type. The field mapping itself is technically correct: camelTo2 '_' correctly converts caseDetailsLink → case_details_link, numberOfCases → number_of_cases, etc.

I'm withdrawing the architectural objection.

What does remain is a parsing robustness concern that exists regardless of approach: every field in CRCVerificationResponse is Maybe, so a field name mismatch between this type and IDfy's actual response will silently produce Nothing with no parse error. This is inherent to the all-Maybe design, not to the single-type approach specifically. If you discover down the road that IDfy returns case_detail_link (no 's') or risk_level instead of risk_type, you will get an all-Nothing response that looks like a successful parse. A comment documenting the exact IDfy field names (with a link to the API spec) would help whoever debugs this first.

Remaining open items

Warning — getTask throws InternalError for in-progress CRC tasks

result <- resp.result & fromMaybeM (InternalError ("Missing result in getTask response: " <> show resp))

When status = "in_progress" and result = null, this throws InternalError on every poll. Court record verification is a slow check (minutes to hours per IDfy docs). Unless callers explicitly avoid polling until status is "completed", this will generate spurious 500s/alerts throughout the wait. Consider at minimum checking resp.status before fromMaybeM to throw a non-alerting pending signal instead.

Warning — Business entity type with ind_court_record has no guard

Passing entityType = Business will produce a 4xx from IDfy at runtime. Please either remove Business from the ADT or add an explicit check before the API call.

Potential — monitor_case removed from request

If case monitoring (ongoing alerts for new case filings) is needed for this feature, this field should be restored.